Skip to content

make default table output a bit more compact #241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jul 7, 2025
Merged

make default table output a bit more compact #241

merged 3 commits into from
Jul 7, 2025

Conversation

shua
Copy link
Contributor

@shua shua commented Apr 30, 2025 •
edited
Loading

I found the default formatting for summarize summarize <file> to be visually noisy and difficult to read because of the separators between each line:

$ summarize summarize my-data.mm_profdata
+-----------------------------------------------------------------+-----------+-----------------+----------+------------+---------------------------------+
| Item                                                            | Self time | % of total time | Time     | Item count | Incremental result hashing time |
+-----------------------------------------------------------------+-----------+-----------------+----------+------------+---------------------------------+
| LLVM_passes                                                     | 714.79ms  | 13.408          | 714.79ms | 1          | 0.00ns                          |
+-----------------------------------------------------------------+-----------+-----------------+----------+------------+---------------------------------+
| run_linker                                                      | 706.56ms  | 13.253          | 706.56ms | 1          | 0.00ns                          |
+-----------------------------------------------------------------+-----------+-----------------+----------+------------+---------------------------------+
| LLVM_module_codegen_emit_obj                                    | 690.58ms  | 12.954          | 690.58ms | 173        | 0.00ns                          |
+-----------------------------------------------------------------+-----------+-----------------+----------+------------+---------------------------------+
| codegen_module                                                  | 401.81ms  | 7.537           | 748.49ms | 172        | 0.00ns                          |

... and so on for like 2 pages of scrolling

This change removes the separating lines +-----+---+ between rows. This introduces a new problem where it is hard to read a line as there's often a lot of whitespace between the end of the item label and the beginning of the next column. I added some ... to pad out the line to make it easier to scan.

$ target/debug/summarize summarize my-data.mm_profdata
+-----------------------------------------------------------------+-----------+-----------------+----------+------------+---------------------------------+
| Item                                                            | Self time | % of total time | Time     | Item count | Incremental result hashing time |
+-----------------------------------------------------------------+-----------+-----------------+----------+------------+---------------------------------+
| LLVM_passes ................................................... | 714.79ms  | 13.408          | 714.79ms | 1          | 0.00ns                          |
| run_linker .................................................... | 706.56ms  | 13.253          | 706.56ms | 1          | 0.00ns                          |
| LLVM_module_codegen_emit_obj .................................. | 690.58ms  | 12.954          | 690.58ms | 173        | 0.00ns                          |
| codegen_module ................................................ | 401.81ms  | 7.537           | 748.49ms | 172        | 0.00ns                          |
| incr_comp_encode_dep_graph .................................... | 302.17ms  | 5.668           | 302.17ms | 223438     | 0.00ns                          |
| implementations_of_trait ...................................... | 280.60ms  | 5.263           | 395.25ms | 44604      | 112.82ms                        |

A similar change has been made for the table output of the summarize diff ... command.

I think the final result is able to present more data in a single screen, while also not sacrificing ease of scanning horizontally.

bjorn3
bjorn3 reviewed Jun 17, 2025
View reviewed changes
bjorn3
bjorn3 reviewed Jun 17, 2025
View reviewed changes
@bjorn3
Copy link
Member

bjorn3 commented Jun 17, 2025 •
edited
Loading

This doesn't seem to have any effect for me locally.
Edit: Seems like you only changed the summarize subcommand, not the diff subcommand. Please also make this change to the diff subcommand.

@shua shua changed the title make default summarize table output a bit more compact make default table output a bit more compact Jun 18, 2025
@shua shua requested a review from bjorn3 June 18, 2025 13:00
bjorn3
bjorn3 reviewed Jun 18, 2025
View reviewed changes
bjorn3
bjorn3 reviewed Jun 18, 2025
View reviewed changes
@shua shua requested a review from bjorn3 June 19, 2025 06:44
@TechPizzaDev
Copy link

A quick heads up that this is getting very close to the format of tables in Markdown, where it would look like this (replace + with | and remove ... for padding):

| Item                          | Self time | % of total time | Time     | Item count | Incremental result hashing time |
|-------------------------------|-----------|-----------------|----------|------------|---------------------------------|
| LLVM_passes                   | 714.79ms  | 13.408          | 714.79ms | 1          | 0.00ns                          |
| run_linker                    | 706.56ms  | 13.253          | 706.56ms | 1          | 0.00ns                          |
| LLVM_module_codegen_emit_obj  | 690.58ms  | 12.954          | 690.58ms | 173        | 0.00ns                          |
| codegen_module                | 401.81ms  | 7.537           | 748.49ms | 172        | 0.00ns                          |
| incr_comp_encode_dep_graph    | 302.17ms  | 5.668           | 302.17ms | 223438     | 0.00ns                          |
| implementations_of_trait      | 280.60ms  | 5.263           | 395.25ms | 44604      | 112.82ms                        |
Item Self time % of total time Time Item count Incremental result hashing time
LLVM_passes 714.79ms 13.408 714.79ms 1 0.00ns
run_linker 706.56ms 13.253 706.56ms 1 0.00ns
LLVM_module_codegen_emit_obj 690.58ms 12.954 690.58ms 173 0.00ns
codegen_module 401.81ms 7.537 748.49ms 172 0.00ns
incr_comp_encode_dep_graph 302.17ms 5.668 302.17ms 223438 0.00ns
implementations_of_trait 280.60ms 5.263 395.25ms 44604 112.82ms

@Kobzol
Copy link
Member

Kobzol commented Jul 4, 2025

I like the compact format, but having it as a Markdown table would be even better.. :)

@shua
Copy link
Contributor Author

shua commented Jul 7, 2025 •
edited
Loading

I could make another PR to output github-flavoured-markdown (gfmd) tables.

Here I'm a bit biased toward improving the plain text version. Notably I included the trailing ... because I found it hard to read long lines in the terminal without any visual line, but those trailing ... would look strange when rendered from gfmd to html. gfmd tables are usually styled in html to have both borders between rows and alternating colours per row which solves this horizontal-scanning problem in a different way.

Kobzol
Kobzol approved these changes Jul 7, 2025
View reviewed changes
Copy link
Member

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's keep it like this for now, I think that this is indeed a readability improvement. Thank you for working on this! I really hope that no one was parsing the text output programmatically.. 😆

@Kobzol Kobzol enabled auto-merge July 7, 2025 07:10
@Kobzol
Copy link
Member

Kobzol commented Jul 7, 2025

@bjorn3 Do you have further comments? You were requested for a review, so GH doesn't allow me to merge the PR until you resolve that :)

@Kobzol Kobzol disabled auto-merge July 7, 2025 07:10
@Kobzol Kobzol enabled auto-merge July 7, 2025 07:15
@Kobzol
Copy link
Member

Kobzol commented Jul 7, 2025

I think this repo is configured to require PRs to be up-to-date, so I rebased your PR on top of master through the GH UI.

@Kobzol Kobzol merged commit 15172a6 into rust-lang:master Jul 7, 2025
4 checks passed
@shua shua deleted the compact branch July 7, 2025 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kobzol Kobzol Kobzol approved these changes

@bjorn3 bjorn3 bjorn3 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shua @bjorn3 @TechPizzaDev @Kobzol